Build/Test Tools: Use strict assertions in the XML-RPC tests. - #13503
Build/Test Tools: Use strict assertions in the XML-RPC tests.#13503haritpanchal wants to merge 2 commits into
Conversation
Replaces the loose assertions in tests/phpunit/tests/xmlrpc with type-strict counterparts, so the tests also assert the type of each value. The XML-RPC server deliberately returns IDs as strings, so that they cannot exceed what an XML-RPC integer can describe, and the expected values are cast accordingly to document that contract. Two assertions are left loose on purpose, with comments explaining why. Also corrects test_ignore_email_change(), which read a WP_User property that does not exist and so passed regardless of the behaviour under test. See #64895.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
lancewillett
left a comment
lancewillett
left a comment
There was a problem hiding this comment.
The overall approach looks sound. Please add the explicit type guards identified inline in the two ignored-ID tests and getTerm() before landing.
Local Docker validation passed: 348 tests / 1,308 assertions in default order and two seeded orders; multisite passed 349 tests / 1,312 assertions. PHPCS passed all 15 changed files.
The user_email correction also held up to a production mutation: accepting the submitted email failed the corrected test, while the original assertion still passed.
Adversarial review · gpt-6
Add explicit assertIsString() guards alongside the strict assertions, so the string return contract is enforced rather than implied. assertNotSame() passes on a type mismatch, and assertStringMatchesFormat() coerces an integer argument, so neither caught an ID returned as an integer. See #64895.
|
Thanks — all three applied. Reproduced both findings by mutation first:
|
Use type-strict comparisons for XML-RPC results and persisted values, matching the documented string ID contracts. Add explicit string checks where negative comparisons and numeric-format assertions do not enforce types. Correct the profile email test to inspect user_email, so it detects an unintended email change. Retain the loose term-array comparison because the internal and XML-RPC representations use different ID types. Developed in: #13503 Props haritpanchal. See #64895. git-svn-id: https://develop.svn.wordpress.org/trunk@63606 602fd350-edb4-49c9-b593-d223f7449a82
github-project-automation
Bot
moved this from In progress
to Done
in WordPress Project Build Tooling
Use type-strict comparisons for XML-RPC results and persisted values, matching the documented string ID contracts. Add explicit string checks where negative comparisons and numeric-format assertions do not enforce types. Correct the profile email test to inspect user_email, so it detects an unintended email change. Retain the loose term-array comparison because the internal and XML-RPC representations use different ID types. Developed in: WordPress/wordpress-develop#13503 Props haritpanchal. See #64895. Built from https://develop.svn.wordpress.org/trunk@63606 git-svn-id: http://core.svn.wordpress.org/trunk@62782 1a063a9b-81f0-0310-95a4-ce76da25c4cd
2 participants
Replaces the loose assertions in
tests/phpunit/tests/xmlrpcwith type-strictcounterparts, so the tests assert the type of each value as well as its value.
43 assertions across 15 files: 40
assertEquals()and 3assertNotEquals().This is the
xmlrpcdirectory claimed on the ticket. None of the three openpull requests (#12583, #11707, #12613) touch this directory, so there is no
overlap with work already in review.
The contract that shapes most of this
XML-RPC deliberately returns IDs as strings, so that they cannot exceed what an
XML-RPC integer can describe.
_prepare_term()states it directly:Accordingly, in
class-wp-xmlrpc-server.php:_prepare_user()→user_id(string)_prepare_post()→post_id,post_parent(string)_prepare_media_item()→attachment_id(string)_prepare_term()→term_id,term_group,term_taxonomy_id,parent(string)_prepare_term()→count(int)_prepare_post()→menu_order(int)wp_newPost(),mw_newPost()return value(string) $post_idSo
assertSame( (string) $id, ... )here documents an intentional API contractrather than papering over a type bug. These are not casts added to make a
conversion pass: the tests already assert the same thing themselves, via
pre-existing
assertIsString( $result['post_id'] )andassertStringMatchesFormat( '%d', $result['term_id'] )lines that pass on trunktoday.
wp/editPost.phplikewise already usedassertSame( '', get_post_meta( $post_id, '_thumbnail_id', true ) ).Values read back from the database follow the same rule and are treated the same
way:
WP_Post::$post_authoris documented@var string(
@phpstan-var numeric-string|'', default'0'), andget_post_meta()returnsthe stored string.
One docblock is imprecise and worth flagging rather than relying on:
WP_Comment::$comment_IDand$comment_post_IDare documented as@var string|int._prepare_comment()passes them through with no cast, so thetype comes from the database — strings. The conversions there rest on the file's
own existing
assertIsString()assertions, not on that union.Two findings, not conversions
1.
wp/editProfile.php—test_ignore_email_change()was asserting nothing.It read
$user_data->email.WP_Userhas noemailcolumn — it isuser_email— andemailis not inWP_User::$back_compat_keys, so__get()fell through to
get_user_meta( $editor_id, 'email', true )and returned''.The assertion was therefore
'notaneditor@example.com' !== '', which is true nomatter what
wp_editProfile()does with the submitted email. The test couldnever have caught a regression in the behaviour its name describes.
Changed to
user_email, which is the property that holds the value under test.Confirmed by mutation: with the old expression the actual value is literally
'', and with the fix a flipped assertion fails against the real storedaddress.
2.
wp/getTerm.php— one assertion stays loose, deliberately.In
test_valid_term(),$termcomes fromget_term( ..., ARRAY_A )withinteger
term_id/parent, while$resultis the prepared struct with those asstrings. A strict comparison of the two arrays would be wrong, so
assertEquals()stays, with a comment recording why, and the individual typescontinue to be asserted on the lines below.
The arguments were also reversed — the actual value was being passed in the
expected position — so that is now
assertEquals( $term, $result ).wp/editTerm.phpgoes the other way for the same reason:WP_Term::$parentis@var int, soassertEquals( '0', $term->parent )was comparing against astring and is now
assertSame( 0, $term->parent ).Testing
The whole group is run, not just the changed files, to confirm nothing
collateral broke.
composer lintis clean on all 15 files, andphp -lpasses on each.Note for anyone reproducing this:
npm run test:php -- tests/phpunit/tests/xmlrpcreports
No tests executed!and exits 0. PHPUnit's default test-file suffix is*Test.php, and thesuffix=".php"inphpunit.xml.distapplies only to theconfigured
<testsuite>, not to a positional directory argument — so adirectory path silently collects zero tests from this suite.
--group xmlrpc(or explicit file paths) is the working form.
Test-only change; no production code is touched.
Trac ticket: https://core.trac.wordpress.org/ticket/64895
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Inventorying the loose assertions, reading each prepared-struct field
back to its cast in
class-wp-xmlrpc-server.phpto decide the expected type perassertion, and drafting this description. I verified every conversion against the
server source and the declared property types, confirmed the three open pull
requests do not overlap this directory, established the
editProfile.phpfindingfrom
WP_User::$back_compat_keysand__get()and confirmed by mutation thatthe replacement assertion can actually fail, ran the full
xmlrpcgroup pluslint and
php -l, and I take responsibility for the result.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.